-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the incorrect size calculation logic of FileSink. #9429
Conversation
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wypb Thank you for the fix.
@xiaoxmeng Meng, would you help review this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wypb Looks great % some nits.
07944c7
to
cb25213
Compare
@mbasmanova sure, will do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wypb good catch. Thanks for the fix % minors!
buffers.back().append(chars[i]); | ||
} | ||
|
||
EXPECT_EQ(buffers.size(), 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: ASSERT__EQ put expected value second while EXPECT_EQ put the expected value first. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified it to ASSERT__EQ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wypb thanks for the update % one more nit for test!
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@xiaoxmeng merged this pull request in d4f8d85. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…tor#9429) Summary: I was writing unit tests for the Textfile writer internally and found that data written to `MemorySink` may be overwritten. I investigated the reason and found that `FileSink::writeImpl` can accept multiple buffers. https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L65-L80 Every time the data of a buffer is written, we should update `FileSink#size_` instead of updating after writing all buffers. Because `MemorySink::write` relies on `FileSink#size_` to write data to `MemorySink#data_`. https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L184-L189 CC: mbasmanova xiaoxmeng Pull Request resolved: facebookincubator#9429 Reviewed By: amitkdutta, kewang1024 Differential Revision: D56004384 Pulled By: xiaoxmeng fbshipit-source-id: c824c5e481866abef12f99d570d41a852bb8b2e0
…tor#9429) Summary: I was writing unit tests for the Textfile writer internally and found that data written to `MemorySink` may be overwritten. I investigated the reason and found that `FileSink::writeImpl` can accept multiple buffers. https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L65-L80 Every time the data of a buffer is written, we should update `FileSink#size_` instead of updating after writing all buffers. Because `MemorySink::write` relies on `FileSink#size_` to write data to `MemorySink#data_`. https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L184-L189 CC: mbasmanova xiaoxmeng Pull Request resolved: facebookincubator#9429 Reviewed By: amitkdutta, kewang1024 Differential Revision: D56004384 Pulled By: xiaoxmeng fbshipit-source-id: c824c5e481866abef12f99d570d41a852bb8b2e0
I was writing unit tests for the Textfile writer internally and found
that data written to
MemorySink
may be overwritten.I investigated the reason and found that
FileSink::writeImpl
can accept multiple buffers.https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L65-L80
Every time the data of a buffer is written, we should update
FileSink#size_
instead of updating after writing all buffers. Because
MemorySink::write
relies on
FileSink#size_
to write data toMemorySink#data_
.https://github.com/facebookincubator/velox/blob/main/velox/dwio/common/FileSink.cpp#L184-L189
CC: @mbasmanova @xiaoxmeng